[BUG] Fixes to S3 Native Lister with correct Error propagation #1401
Conversation
samster25
commented
Sep 21, 2023
•
samster25
commented
- Fixes s3 listing infinte loop
- correct error propagation in the recursive file lister
- return directories in the recursive file lister
- Fixes bug when you pass in an exact file
- Adds tests to cover the above cases
- Adds s3fs recursive lister utility
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1401 +/- ##
==========================================
+ Coverage 87.78% 87.80% +0.01%
==========================================
Files 60 60
Lines 6036 6041 +5
==========================================
+ Hits 5299 5304 +5
Misses 737 737
|
samster25
changed the title
| FileType::File => tx.send(tr).await.unwrap(), | ||
| FileType::Directory => add_to_channel(source.clone(), tx.clone(), tr.filepath), | ||
| }; | ||
| let tr = tr; |
There was a problem hiding this comment.
This line appears to be redundant now.
There was a problem hiding this comment.
| .map(|k| k.strip_suffix('/').unwrap_or(k)); | ||
| let key = key.unwrap_or(""); | ||
|
|
||
| let key = key.strip_prefix('/').unwrap_or(""); |
There was a problem hiding this comment.
Hmm this seems like this will coerce the key to an empty string if it doesn't have a "/" prefix (i.e. if it isn't a base URL); for example, it appears that "foo/bar" would be coerced to "". Do we have a guarantee that all provided URLs will contain base URLs when URL-parsed?
Shouldn't this be .unwrap_or(key) just to be safe?
There was a problem hiding this comment.
The UrlParse library leaves the prefix when your parse the url so "foo/bar" is hostname="foo", key="/bar". so if theres no prefix then it means it's an empty string.
| } else { | ||
| let key = key.strip_suffix('/').unwrap_or(key); | ||
| format!("{key}/") | ||
| }; |
There was a problem hiding this comment.
Seems like this might be more simply expressed as:
let key = key.strip_prefix('/').unwrap_or(key);
let key = if !key.ends_with('/') { format!("{key}/") } else { key };There was a problem hiding this comment.
What I wanted to take care of here is to drop trailing slashes but ensure that there's atleast one. looks like the right call was actually trim_** in https://github.com/Eventual-Inc/Daft/pull/1404/files
